fix(alias,auto-install,babel,beep,buble,commonjs,data-uri,dsv,dynamic-import-vars,eslint,esm-shim,graphql,html,image,inject,json,legacy,multi-entry,node-resolve,pluginutils,replace,run,strip,sucrase,swc,terser,typescript,url,virtual,wasm,yaml): ensure rollup 4 compatibility #1595

lukastaegert · 2023-09-27T08:00:27Z

Rollup Plugin Name: `alias,auto-install,babel,beep,buble,commonjs,data-uri,dsv,dynamic-import-vars,eslint,esm-shim,graphql,html,image,inject,json,legacy,multi-entry,node-resolve,pluginutils,replace,run,strip,sucrase,swc,terser,typescript,url,virtual,wasm,yaml`

This PR contains:

Are tests included?

yes (well, adapted...)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Rollup 4 is nearly ready for release. As a matter of fact I am hoping for a release by Thursday next week (i.e. at ViteConf). However, this PR would need to be merged first as otherwise, plugins are not usable for Rollup 4 users.

There should be no breaking changes in this release.

This is a list of all changes:

Updated the peerDependency versions of Rollup to include ^4.0.0
Replaced assertions with attributes in test snapshots of resolve options as that was a major change. Apparently, it does not affect any plugins, though, as they ignore that attribute.
commonjs
- As parse errors no longer contain a loc attribute but only a pos attribute, the logic was changed to forward this attribute to Rollup on errors instead of loc. This is non-breaking as earlier Rollup versions already had pos as well.
- Added explicit skipSelf: false to this.resolve calls where unspecified as the default value has changed. Again, this is non-breaking as it just explicitly states the previous default.
- Explicitly skip transforming the value after parsing the key of a shorthand property. Before, they were only deep equal while now, they are the same reference. This trips up is-reference, but I think this is the best way to resolve this. Again, this should have no effect on earlier Rollup versions.
Updated some snapshots to account for minor changes, e.g. Errors no longer have an own toString method.
node-resolve
- added explicit skipSelf: false to this.resolve calls where unspecified as the default value has changed.
typescript
- replaced RollupWarning type with a custom RollupLog type containing only the properties we need. Once we drop rollup@2 support, this can be replaced by the real RollupLog type again. This avoids a breaking change. I added a corresponding TODO comment.
- Added some type casts to support querying inputOptions.preserveModules on older Rollup versions, even though this property has been removed now. I added a TODO comment to remove the corresponding check once we dropped rollup@3 support.

@shellscape I hope I got the commit message right this time 😉

tada5hi

The failing windows ci should not be related to the changes

lukastaegert · 2023-09-28T04:20:45Z

The failing windows ci should not be related to the changes

That would be a relief, but why is it failing? Is it broken on master?

tada5hi · 2023-10-03T12:56:03Z

@lukastaegert think another consideration we could make is to set the setting --workspace-concurrency to 1. What do you think ?

shellscape · 2023-10-03T13:29:16Z

For the Windows build it seems like a good idea. We should also report this to pnpm

lukastaegert · 2023-10-03T19:41:02Z

Forcing ppm to 8.8.0 did not solve the issue. Adding --workspace-concurrency 1 to the Windows test run works, though, as you can see.

lukastaegert · 2023-10-04T04:40:04Z

I slightly reworked the changes in the TypeScript plugin so that there is no change in the productive code at all and no potential for breakage. I also added two TODO comments to change the code once we dropped support for rollup@2 and rollup@3 respectively.

@shellscape Do you see any chance we can merge this tomorrow? This would allow me to release Rollup 4 in time for ViteConf. The productive changes are now very minimal and non-breaking, otherwise it mostly concerns test code and snapshots, see the list in the PR description.

shellscape · 2023-10-04T04:46:07Z

I'm going to be out of pocket for the rest of the week with travel unfortunately. BUT I've been following along and I see no reason not to push it forward. You've got my backing.

lukastaegert · 2023-10-04T04:48:01Z

Perfect, thanks so much!

sapphi-red · 2023-10-04T07:39:48Z

While trying out Rollup v4 on the Vite repo (I didn't use the plugins built on this PR's branch), I found that the rebuild doesn't happen when I change the file while running rollup --watch. I guess this change (rollup/rollup#5150) now requires the typescript plugin to call this.addWatchFile.

lukastaegert · 2023-10-05T06:44:26Z

@sapphi-red Great catch! I added this and a test, should be working now!

…-import-vars,eslint,esm-shim,graphql,html,image,inject,json,legacy,multi-entry,node-resolve,pluginutils,replace,run,strip,sucrase,swc,terser,typescript,url,virtual,wasm,yaml): ensure rollup 4 compatibility

sapphi-red · 2023-10-05T07:21:57Z

Thanks! I have tried it locally on the Vite repo and confirmed that it works. 👍

lukastaegert · 2023-10-05T12:40:38Z

Ok, it seems there was a hiccup with GitHub during release. Essentially, all packages before sucrase were successful but all others were not published. Worse, the changelog commit was also not generated. I am wondering how to best remedy this, but I guess I should leave it as it is. A subsequent publish run would fail as well as it would try to publish over the already successfully published versions.
In order to have the Rollup release unblocked, I would manually release patch versions of the remaining packages without updating the repository. Then the error is at least symmetric across all packages, i.e. every package is one patch version behind, and we will need to see how to clean up the repo later and update the changelog.

lukastaegert · 2023-10-05T13:01:26Z

Ok, sucrase seems to be in a weird state. npm show @rollup/plugin-sucrase versions says that 5.0.1 is the latest version, but publishing 5.0.2 fails because it thinks that the version already exists. All other plugins are updated but as I said, the changelog is not updated. I will leave it in that state.

tada5hi · 2023-10-05T13:02:05Z

Ok, it seems there was a hiccup with GitHub during release. Essentially, all packages before sucrase were successful but all others were not published. Worse, the changelog commit was also not generated. I am wondering how to best remedy this, but I guess I should leave it as it is. A subsequent publish run would fail as well as it would try to publish over the already successfully published versions.

In order to have the Rollup release unblocked, I would manually release patch versions of the remaining packages without updating the repository. Then the error is at least symmetric across all packages, i.e. every package is one patch version behind, and we will need to see how to clean up the repo later and update the changelog.

Another possibility would be to merge pull request #1585, since this also targets all repos. However, this would not solve the problem that for some packages no previous version exists.

lukastaegert · 2023-10-05T13:31:04Z

Exactly. Maybe the solution would be to just manually add a commit that raises all patch versions by 1 to master, and THEN merge the other PR. I would assume that this would then generated the changelog containing the entries from both PRs. But I would prefer to wait until @shellscape is back to have a look.

shellscape · 2023-10-08T14:24:59Z

I haven't been able to closely follow along, so not entire sure how we got into the situation. I'll take a look at this today to sort it out.

OK after looking at the state of things, this is a real mess. Each plugin is going to have to be checked and have things manually updated. That's going to be a lift. Moving forward, we should wait until we have someone around who is intimately familiar with the release process before pushing a big one out like this. (And I would prefer that to be more than just myself)

Or, do smaller groups of changes to contain.

shellscape · 2023-10-08T15:10:14Z

@lukastaegert did you manually publish these? The workflow log doesn't show any publishing activity other than a failure on alias, which should have subsequently stopped anything else from being published.

shellscape · 2023-10-08T15:42:22Z

OK all of the changelogs and package.json version properties are updated, and I've tagged the repo with the versions as well. We should be in good shape. I'll be looking into process hardening in the near future here.

lukastaegert requested review from tada5hi, tjenkinson, Andarist and shellscape as code owners September 27, 2023 08:00